Fix missing SVGs from barrel re-exports and unbuilt local SVG map#1329
Draft
jonathanmos wants to merge 10 commits into
Draft
Fix missing SVGs from barrel re-exports and unbuilt local SVG map#1329jonathanmos wants to merge 10 commits into
jonathanmos wants to merge 10 commits into
Conversation
Member
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dce270e341
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
sbarrio
approved these changes
Jul 8, 2026
jonathanmos
force-pushed
the
jmoskovich/fix-svg-missing-in-replay
branch
from
July 26, 2026 10:14
499852d to
e82aa14
Compare
🎉 All green!🧪 All tests passed 🔗 Commit SHA: ac44218 | Docs | Datadog PR Page | Give us feedback! |
SvgViewMapper returned an empty wireframe list when an SVG's hash or resource entry data was unavailable, making the element fully invisible in the replay instead of showing at least its bounding box.
buildSvgMap() is a no-op until setApiTypes() has been called (it guards on this.t). Both the plugin's pre() hook and the generate-sr-assets CLI called buildSvgMap() first, so the project-wide SVG scan silently produced an empty map on every real build.
For `export { default as Logo } from './icon.svg'`, the scan keyed
the map entry on spec.local.name ('default') instead of spec.exported
('Logo') -- the name consumers actually import and render as <Logo/>.
Aliased re-exports were therefore never matched.
pre() assigned options.__internal_reactNativeSVG directly with no fallback, so in a real build (no injected instance) every file discarded the previous scan and rebuilt/rescanned from scratch.
HandlerResolver/LocalSvgHandler matched purely on JSX tag name against a project-wide map, with no check that the current file actually imported that name from an SVG. A plain component <Icon/> in one file could get wrapped as an unrelated SVG also named Icon imported or re-exported elsewhere in the project. resolveSvgImport now resolves each usage site through the JSX tag's real scope binding, tracing it to its source module (direct .svg import or barrel re-export via svgFileMap) before treating it as SVG.
Gating processItem() on state.filename being truthy skipped all SVG handling -- including the <Svg> tag path, which never needed a filename -- whenever filename was falsy. resolveSvgImport already fails closed on its own when given an empty currentFile.
The package-root computation was a hardcoded ../../../.. offset from __dirname, correct only when running from the built lib/commonjs/... output. Under ts-jest or ts-node it resolved one directory too high, into the shared monorepo directory instead of this package's own root. resolvePackageRoot now walks up to find this package's actual package.json.
jonathanmos
force-pushed
the
jmoskovich/fix-svg-missing-in-replay
branch
from
July 26, 2026 11:09
e82aa14 to
c80d43c
Compare
Every existing test injected __internal_reactNativeSVG pre-built with setApiTypes() already called, bypassing pre()'s own instance construction entirely -- meaning the setApiTypes/buildSvgMap ordering fix could regress without any test catching it. Adds a test that lets pre() build its own instance, and a test that runs the actual generateSessionReplayAssets() CLI entrypoint end-to-end.
A stray svg-map.json at this package's real root (e.g. left behind by a local datadog-generate-sr-assets run) would make every test in this file silently read stale cached data instead of scanning its own tmp fixtures. Hide that one path from fs.existsSync unconditionally.
jonathanmos
force-pushed
the
jmoskovich/fix-svg-missing-in-replay
branch
from
July 26, 2026 12:20
c80d43c to
ae922b2
Compare
References like "Fix 2, 3, 4, 6" only made sense during the session that introduced them and mean nothing to a future reader. Replaced with descriptions of what each case actually verifies.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
SVGs (icons, logos) were silently missing from Session Replay recordings. Root cause: buildSvgMap() never actually ran due to an initialization-order bug, and even when scanning worked, barrel re-exports and cross-file name collisions were resolved incorrectly. This PR fixes the pipeline end-to-end and adds regression coverage for each issue.
Before
After
Changes
Core fixes
Call setApiTypes before buildSvgMap in both the plugin's pre() hook and the generate-sr-assets CLI — buildSvgMap() silently no-op'd without this, meaning the SVG scan never ran in any real build.
Reuse the ReactNativeSVG instance across files in pre() instead of rebuilding (and rescanning) it per file.
Fix barrel re-export naming: export { default as Logo } from './icon.svg' was keyed under 'default' instead of 'Logo', so aliased re-exports were never matched.
Scope local SVG resolution to the importing file's actual bindings — the main fix. Previously a JSX tag was matched by name alone against a project-wide map, so an unrelated component sharing a name with some SVG elsewhere in the project could get wrongly wrapped (or vice versa). Resolution now traces each usage to its real import binding before treating it as an SVG.
Always attempt SVG processing regardless of missing filename (a stricter guard introduced during this work was skipping the tag path unnecessarily).
Fix svg-map.json package-root resolution so it works when running from source (tests, dev) and not just the built package.
Other
Tests
Harden tests against a real local svg-map.json cache file interfering with results.
Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)